-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Tune] Dynamically identify PyTorch Lightning Callback hooks #30045
Conversation
…r modifying deprecated functions), reorganized the order to match the rest of the functions, along with the integration tests. Signed-off-by: Davina Zaman <[email protected]>
…ust tests Signed-off-by: Davina Zaman <[email protected]>
Signed-off-by: Amog Kamsetty <[email protected]>
Signed-off-by: Amog Kamsetty <[email protected]>
Can we test this somehow? |
We already have tests for this @Yard1 |
Would it be possible to test the new logic introduced in this PR, though? Eg. use runtime envs to install 2 PTL versions and see that the hooks were detected correctly? |
Hmm but the logic introduced in this pr is already being tested right? There’s no version specific logic introduced in this pr. Should we be testing all of our integrations against all external library versions? |
I suppose that's true, we don't really have a precedent here. I just find it a bit worrying that the intention of this PR is to make the callback version-independent but we have no way of checking if that was achieved |
I agree, but to be fair, the intention of the callback in general (even before this PR) was also to be version independent. Ideally, there would be a good way to test this, but we end up having to test against lots of different versions. |
Yeah, that's understandable. The logic looks good to me! |
Signed-off-by: Amog Kamsetty <[email protected]>
Failing test is flaky on master, going to merge |
…ject#30045) Our current PTL integration Callback hooks are hard coded and contain deprecated hooks which log a warning: ray-project#27487. Some of these hooks have already been deprecated in PTL 1.6, others are deprecated in PTL 1.7, and others in 1.8. Instead of maintaining a different PTL integration callback for each PTL version, we instead dynamically identify the available hooks for whatever PTL version the user has installed. Signed-off-by: Davina Zaman <[email protected]> Signed-off-by: Amog Kamsetty <[email protected]> Co-authored-by: Davina Zaman <[email protected]> Signed-off-by: Weichen Xu <[email protected]>
Our current PTL integration Callback hooks are hard coded and contain deprecated hooks which log a warning: #27487.
Some of these hooks have already been deprecated in PTL 1.6, others are deprecated in PTL 1.7, and others in 1.8. Instead of maintaining a different PTL integration callback for each PTL version, we instead dynamically identify the available hooks for whatever PTL version the user has installed.
Closes #27487
Closes https://github.com/anyscale/product/issues/14165
Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.